Skip to content

fix: export NetworkFixture class as value#32

Open
christoph-fricke wants to merge 1 commit intomswjs:mainfrom
christoph-fricke:fix/expose-network-fixture-as-value
Open

fix: export NetworkFixture class as value#32
christoph-fricke wants to merge 1 commit intomswjs:mainfrom
christoph-fricke:fix/expose-network-fixture-as-value

Conversation

@christoph-fricke
Copy link

Before version v0.4.3 the class NetworkFixture was exposed as a full export. #17 changed it to a type-only export, which results in the class no longer being exported from JS (only its type definitions file).

In my use-case I need to construct the handlers passed to the network fixture from another Playwright fixture. Therefore, using the provided high-level createNetworkFixture function to create the fixture does not work in this case.

network: [
  async ({ page, store, baseURL }, use) => {
    const worker = new NetworkFixture({
      page,
      initialHandlers: defineHandlers(store, baseURL!),
    });
    await worker.start();
    await use(worker);
    await worker.stop();
  },
  { auto: true },
],

@kettanaito
Copy link
Member

Hi, @christoph-fricke. The NetworkFixture is not intended to be used as a class because you shouldn't be extending it.

You can configure createNetworkFixture with the other fixture's values as input by unwrapping the fixture itself:

{
  network: [
    ({ page, store, baseURL }, use) => {
      const [fixture] = createNetworkFixture({
        initialHandlers: defineHandlers(store, baseURL!)
      })

      return fixture({ page }, use)
    },
    { auto: true }
  ]
}

This might not be the most elegant approach, but that is what we have given the current Playwright fixtures composition. I highly suggest you explore this instead of extending the NetworkFixture class.

Let me know if this works for your case.

@christoph-fricke
Copy link
Author

Hi @kettanaito. I am only creating an instance of the NetworkFixture class not extending it, right? Similar to using setupServer() or setupWorker() in MSW.

While I am open to use your workaround instead and have tried to use it, it breaks instantly once type checking and type-aware linting come into play. Not using destructuring does not work because Playwright fails with "First argument must use the object destructuring pattern.":

network: [
  ({ page, store, baseURL }, use, testInfo) => {
    const [fixture] = createNetworkFixture({
      initialHandlers: defineHandlers(store, baseURL!),
    });
    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
    // @ts-ignore: Don't want to destructure and restructure every Fixture Arg...
    // eslint-disable-next-line @typescript-eslint/no-unsafe-return
    return fixture({ page }, use, testInfo); // `fixture` returns any...
  },
  { auto: true },
],

Compared to to the snippet, creating an instance of NetworkFixture seems to be more robust and feels like a "designed" API.

@kettanaito
Copy link
Member

Thanks for the insight. That's not a great experience, but I would say using NetworkFixture directly isn't one either.

I've seen two approaches to Playwright fixtures in a library context:

  • Export a function that returns an entire fixture (what we do with createNetworkFixture.
  • Export just the core functionality that's "fixture-less" and let the user connect the wires.

The second one sounds more like what you're proposing.

@christoph-fricke
Copy link
Author

I could imagine exporting support for both scenarios. That is how playwright-msw has done it. How about:

  • type NetworkFixture: Just the type to type the fixture definition. Does it need another name?
  • createNetworkFixture: Existing high-level API to conveniently create the whole fixture
  • setupNetwork: Low-level API for creating a network interceptor (instance of NetworkFixture)
    • Similar to setupWorker and setupServer, but uses Playwright for interception.
    • For me, it is closer to setupServer which uses .listen() and .close(). Does it make sense to rename the current .start() and .stop()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants